Add the ByteSize measure #490
Conversation
nfrisby
force-pushed
the
nfrisby/bytesize-measure
branch
from
221714c
to
8defee1
Compare
|
TODO test suite |
| -- case above. | ||
| class ByteSizePartialFrom a where | ||
| -- | See 'ByteSizePartialFrom'. | ||
| partialFrom :: a -> StrictMaybe ByteSize |
There was a problem hiding this comment.
I wouldn't really call it partial, since partial functions in Haskell usually are the ones that produce exceptions, eg. error is a partial function. I get it that math has a different view on the subject, but in order to stick to Haskell nomenclature I'd call it something like toByteSizeMaybe
DefaultSignatures can reduce some boiler plate
| partialFrom :: a -> StrictMaybe ByteSize | |
| partialFrom :: a -> StrictMaybe ByteSize | |
| default partialFrom :: ByteSizeFrom a => a -> StrictMaybe ByteSize | |
| partialFrom = partialFromDefault |
| -- | Law: @'partialFrom' = 'SJust' . 'from'@ | ||
| class ByteSizePartialFrom a => ByteSizeFrom a where | ||
| -- | See 'ByteSizeFrom'. | ||
| from :: a -> ByteSize |
There was a problem hiding this comment.
Not only to and from are very confusing and ambiguous names, but they are also highly likely to be used in user's codebase. Without qualification these will lead to clashes quite often (even lens/microlens has to)
I'd suggest better names that depict what is the actual conversion here. So in order to avoid ambiguity of what is being converted to what, I'd suggest something like this:
-- | A type that can be safely converted to `ByteSize`
class ToByteSize a where
toByteSize :: a -> ByteSize
-- | A type that can be safely constructed from `ByteSize`
class FromByteSize a where
fromByteSize :: a -> ByteSizeThere was a problem hiding this comment.
Without qualification ...
So far, I'm intending for this module to be imported qualified.
Example: ByteSize.from @Word32 1000
There was a problem hiding this comment.
Do we need to export SJust . from at all?
There was a problem hiding this comment.
So far, I'm intending for this module to be imported qualified.
I appreciate your intention, but that should not be forced on the user. Moreover, qualification does not solve the ambiguity of naming from and to.
So, to put it bluntly, I will not let functions with names like from and to to be merged.
| #if __GLASGOW_HASKELL__ < 900 | ||
| -- Use the GHC version here because this is compiler dependent, and only indirectly lib dependent. | ||
| import GHC.Natural (Natural) | ||
| #endif |
There was a problem hiding this comment.
Why CPP here?
| #if __GLASGOW_HASKELL__ < 900 | |
| -- Use the GHC version here because this is compiler dependent, and only indirectly lib dependent. | |
| import GHC.Natural (Natural) | |
| #endif | |
| import Numeric.Natural (Natural) |
There was a problem hiding this comment.
I don't know; I merely copied it from an existing module in the package.
Edit: seems like the CI does not like it 🤔
| -- | NB the result will never be @'SJust' 'maxBound'@. | ||
| instance ByteSizeTo Word64 where to = toBiggerIntegral | ||
|
|
||
| instance ByteSizePartialFrom Natural where partialFrom = partialFromDefault |
There was a problem hiding this comment.
This could overflow, isn't it? Shouldn't we do sth like this here:
\a -> if a >= fromIntegral sentinel then SNothing else ...
The new
ByteSizemeasure should be used for all byte size summations relevant to the Cardano chain.Monoidinstance that will be useful for finger trees.The other types defined in the module should be used for recording/transmitting the result of summations that did not overflow.